I'm against merging version.cr into the main module. Keeping it a separate file is IMO cleaner and would be compatible with Ruby convention which does the same (see rails' version.rb). I see no need to clutter main module with declarations having great time in their own company.

Hello @RX14, thank you for adding this to crystal init. I completely forgot about that feature and sending the PR to use it 🤦‍♂️

I would personally disagree on moving the VERSION constant within the main module code. I believe it adds value beyond the file location itself, but also some orientation in relation to code structure (ie. hey, you can place the files inside src/name/... and will be loaded by require)

Cheers.

@RX14 could you reduce this PR to using shards version? version.cr has it's pros and cons and should probably be discussed separately.

One nice thing about the lib/project/version.rb in Ruby is that the version file can be loaded independently of the full library if all you want is the version. This is especially used in many .gemspec files. Not sure that applies to Crystal projects or not.

In my view version.cr is much more cluttered than just adding a line in your main file. Ruby convention has it separate only because the gemspec depends on the version.cr - in Crystal we have it the other way around so we can merge it back in.

Additionally it means we don't enforce an opinion between

src/
├── example.cr
└── example/
    ├── foo.cr
    └── bar.cr

vs

src/
├── example.cr
├── foo.cr
└── bar.cr

Hello @RX14, thank you for your response.

I think you're basing your opinion in comments like this and this?

(Refs from #2788 and #2690).

If that is the logic that is driving your change, will be great if you can add those details in the commit message, so future developers will be able to find the logic of that decision.

Thank you.

@Wulfklaue please keep this discussion focused on the PR. You could open a new issue for your comment.
But keep in mind: nobody needs to use crystal init, you can bootstrap a new project easily by hand. It's just a tool to get you started with a set of opinionated defaults.

Updated with a new longer commit message.

@RX14 The sentence We can use it here to automatically generate the version constant for libraries directory from shard.yml, meaning it will never be out of sync. looks a bit off. It seems like something is missing - or libraries directory is just not a great term. The linebreak after We should be removed.

It's a typo: directly instead of directory

Fixed typo.

Rebased to fix travis, hope this can get reviewed.

@RX14 With example/version.cr removed it is questionable if the require "./example/*" should stay in example.cr, considering it advocates against a flat file structure.

I initially thought that this meant shards version would be executed once for each shard, but this is only true if the VERSION constant is actually referred and used in code, because of the laziness of constants. That laziness might disappear one day, though.

That said, I don't know how much time it takes to run, say, 100 or 200 of those shell commands (we should profile this). Maybe it's negligible.

I personally don't think keeping the version in shards.yml and VERSION such a hard task. So I have mixed feelings regarding this change.

@asterite this is a macro, surely the constant laziness doesn't apply here? (only runtime)

Personally, I've thought about this some more and I think VERSION constants actually aren't that useful. I've never used a VERSION constant in somebody else's library, though I can see how it's useful.

I think a much better system to provide would be to provide shards get-version shard_name. That way, applications that actually need the version of a shard can retrieve it using macros, but that burden is on the user of the version information, not the provider.

Another option is just to generate by default a spec similar to this one.

I like the idea of simply having a spec for the version. Essentially, it moves reading shard.yml from every build to only on (full) spec suite runs.

@RX14 What I mean with constant laziness is that the value of constants aren't typed unless used. Try this:

ONE = 1 + 'a'
TWO = {{ 1 + 'a' }}

it compiles. It's only when you refer to those constants in code that you'll get an error.

@asterite I wouldn't have thought that that worked with macros, I would have assumed that TWO = {{ 1 + 'a' }} would be a top-level macro exactly equivalent to

{% begin %}
  TWO = {{ 1 + 'a' }}
{% end %}

I thought macros are always done in a pass before we even know which constants are used, so they'd just be evaluated all the time. I understand that 1 + 'a' won't be typed and will never fail, but wouldn't have thought that macro evaulation behaved the same way.

Appears i'm incorrect.

Rebased onto master to fix merge conflict.

Anything left to do here if this doesn't pose a performance impact?

Relying on subprocesses at compile time is bad.
Relying on external executables is bad.
Relying on PATH is horrible.
Putting this monstrosity in every library's code is a pandora's box.

Yeah I think we should rather keep it as is for now instead of introducing a feature into potentially many libraries that might prove to pose more trouble than anything.

I think it would be more feasible if the compiler would read the data directly from the shard.yml but should not rely on shards CLI being available in PATH.

But I like the idea of removing the extra file for the version constant.

Can't we just use a makeshift "YAML parser" like this?

File.read_lines("#{__DIR__}/../shard.yml").find(&.starts_with?("version:")).try &.byte_slice(8).partition('#')[0].delete(%("')).strip

Maybe put in a method Crystal.shards_version with some additional logic to correctly resolve shard.yml path relative to calling file.

I'll vote to close: as said in my comment, moving crystal init to the existing shards init will prevent us from a mess resulting from a mutual dependence of crystal <=> shards

@j8r that's a different path and probably a good idea, but both changes are not mutually exclusive. It doesn't make a difference if the file is created by crystal init or shards init, extracting the version from shard.yml (on every compilation) shouldn't depend on the shards command anyway.

It seems this got stalled on a dispute on whether we should make all new projects generated with crystal init depend on the shards command being available.

While one could argue that's already the case as soon as you add a single dependency, I can see the point. Given this doesn't seem to really fix a pain point as evidenced by the lack of discussion and voting on this and no issues referencing this, I'll close this for now.